Fix bug in Bytes.lastIndexOf when array is empty and position is not 2²⁵⁶-1 #5797
Conversation
🦋 Changeset detectedLatest commit: 6eb86f4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Amxx
force-pushed
the
bugfix/Bytes-lastIndexOf-empty
branch
from
6d20d77 to
edf974e
Compare
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
|
This fix looks good to me. |
|
@ernest @hadrien cc @levi The fix LGTM. Just a nit: using the ternary operator, although correct, is not super readable IMO i.e., the logic is not immediately clear. Have you considered the following simpler syntax instead? uint256 end;
if(pos == type(uint256).max) {
end = length;
} else {
end = Math.min(pos + 1, length);
} |
|
The if / else syntax generates a conditional jump that is quite expensive. So does the ternary operator Math.ternary does the same using xor and mul opcode that are really cheap. That is why we favor that (branchless) syntax when possible |
contracts/utils/Bytes.sol
Outdated
| @@ -58,8 +58,8 @@ library Bytes { | |||
| function lastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) internal pure returns (uint256) { | |||
| unchecked { | |||
| uint256 length = buffer.length; | |||
| // NOTE here we cannot do `i = Math.min(pos + 1, length)` because `pos + 1` could overflow | |||
| for (uint256 i = Math.min(pos, length - 1) + 1; i > 0; --i) { | |||
| uint256 end = Math.ternary(pos == type(uint256).max, length, Math.min(pos + 1, length)); | |||
There was a problem hiding this comment.
This might be more readable, and just as effective.
| uint256 end = Math.ternary(pos == type(uint256).max, length, Math.min(pos + 1, length)); | |
| uint256 end = Math.min(Math.saturatingAdd(pos, 1), length); |
@ernestognw @vesselinux @levi-sledd WDYT ?
There was a problem hiding this comment.
This is indeed equivalent and more readable.
There was a problem hiding this comment.
@Amxx Ahh, sorry to say but I find this to be even less readable haha. That's maybe because the overflow logic is buried inside the saturatingAdd thus making handling of edge cases less obvious. At the same time, I fully accept the arguments against my if-else suggestion.
The above said, I appreciate that readability is subjective, so feel free to adopt whichever of the two suggested variants the Contracts team like best. Indeed both are functionally equivalent and that's what matters at the end of the day.
There was a problem hiding this comment.
the overflow logic is buried inside the saturatingAdd thus making handling of edge cases less obvious
Interesting point of view. imo "burying the logic" should be the goal of the saturatingAdd function (and generally for any other library fn). In fact, I do prefer the saturatingAdd given I'd recommend using it to other developers in a similar situation as this.
…2²⁵⁶-1 (#5797) Co-authored-by: Ernesto García <ernestognw@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
…2²⁵⁶-1 (#5797) Co-authored-by: Ernesto García <ernestognw@gmail.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Fix bug introduced in #5252 (v5.2)
Bug description
If the
bufferis empty and ifposis nottype(uint256).maxMath.min(pos, length - 1) + 1,length-1overflow and return2²⁵⁶-1.posand that will return something that is NOT2²⁵⁶-1.This will return unpredictable results, if s can be found after the buffer. For example, if the memory after the is clean (zero) and you do
lastIndexOf('0x', '0x00', 17)... it will return 17 instead of the expected 2²⁵⁶-1 (not found)PR Checklist
npx changeset add)